-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fixed updates of excluded prefixes #1180
fixed updates of excluded prefixes #1180
Conversation
80e48a3
to
7a8881c
Compare
prefixPool atomic.Value | ||
once sync.Once | ||
configPath string | ||
previousPrefixes map[string]struct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain why is this not per connection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as far as I can see these changes are still not resistant to the following scenarios:
- Client changed his prefixes on refresh.
- Client's and server's prefixes have a collision on the first request and do not have a collision on the next request. request1: {clientPrefixes: a, serverPrefixes:a, finalPrefixes: a}, Actual: request2: {clientPrefixes: a, serverPrefixes:b, finalPrefixes: b}.
Expected: request2: {clientPrefixes: a, serverPrefixes:b, finalPrefixes: ab}.
@denis-tingaikin reworked some logic, added unit test for describe scenario - please, take a look |
prefixesFromFile := eps.prefixPool.Load().(*ippool.PrefixPool).GetPrefixes() | ||
// excluded prefixes from client | ||
clientPrefixes := conn.GetContext().GetIpContext().GetExcludedPrefixes() | ||
|
||
sort.Strings(prefixesFromFile) | ||
sort.Strings(clientPrefixes) | ||
|
||
if IsEqual(prefixesFromFile, clientPrefixes) { | ||
return next.Server(ctx).Request(ctx, request) | ||
} | ||
|
||
prefixesInfo, _ := load(ctx) | ||
|
||
clientPrefixesChanged := !IsEqual(prefixesInfo.previousClientPrefixes, clientPrefixes) | ||
filePrefixesChanged := !IsEqual(prefixesInfo.previousFilePrefixes, prefixesFromFile) | ||
|
||
finalPrefixes := clientPrefixes | ||
if !clientPrefixesChanged && filePrefixesChanged { | ||
finalPrefixes = removePrefixes(clientPrefixes, prefixesInfo.previousDiff) | ||
} | ||
|
||
finalPrefixes = removeDuplicates(append(finalPrefixes, prefixesFromFile...)) | ||
request.GetConnection().GetContext().GetIpContext().ExcludedPrefixes = finalPrefixes | ||
|
||
if filePrefixesChanged { | ||
prefixesInfo.previousFilePrefixes = make([]string, len(prefixesFromFile)) | ||
copy(prefixesInfo.previousFilePrefixes, prefixesFromFile) | ||
} | ||
|
||
if clientPrefixesChanged { | ||
prefixesInfo.previousClientPrefixes = make([]string, len(finalPrefixes)) | ||
copy(prefixesInfo.previousClientPrefixes, finalPrefixes) | ||
} | ||
|
||
prefixesInfo.previousDiff = Subtract(clientPrefixes, prefixesFromFile) | ||
|
||
log.FromContext(ctx). | ||
WithField("excludedPrefixes", "server"). | ||
WithField("prefixesFromFile", prefixesFromFile). | ||
WithField("previousPrefixesInfo", prefixesInfo). | ||
Debugf("adding excluded prefixes to connection") | ||
|
||
store(ctx, prefixesInfo) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it starts to look more and more complicated.
I think we should consider the problem on the client(nsc) side within exludeprefix.Client chain element that will simply translate response prefixes to client prefixes. In this case all applications in the chain of request can always append prefixes and don't care about diff/conflict cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@denis-tingaikin ok, it sounds like a good solution, added it
@Mixaster995 Could you rebase this? |
Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
fb9f391
to
90d3615
Compare
Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
90d3615
to
1b302ad
Compare
@denis-tingaikin ok, done |
// IsEqual check if two slices contains equal strings, no matter the order | ||
func IsEqual(s1, s2 []string) bool { | ||
s1copy := make([]string, len(s1)) | ||
s2copy := make([]string, len(s2)) | ||
|
||
copy(s1copy, s1) | ||
copy(s2copy, s2) | ||
|
||
sort.Strings(s1copy) | ||
sort.Strings(s2copy) | ||
|
||
return reflect.DeepEqual(s1copy, s2copy) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using github.com/google/go-cmp/cmp
We already have dep on it https://github.com/networkservicemesh/sdk/blob/main/go.mod#L17
// IsEqual check if two slices contains equal strings, no matter the order | |
func IsEqual(s1, s2 []string) bool { | |
s1copy := make([]string, len(s1)) | |
s2copy := make([]string, len(s2)) | |
copy(s1copy, s1) | |
copy(s2copy, s2) | |
sort.Strings(s1copy) | |
sort.Strings(s2copy) | |
return reflect.DeepEqual(s1copy, s2copy) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@denis-tingaikin ok, done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Only one comment to fix and this can be merged.
Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
…k@main PR link: networkservicemesh/sdk#1180 Commit: eefb4a2 Author: Авраменко Михаил Date: 2021-12-24 19:34:52 +0700 Message: - fixed updates of excluded prefixes (#1180) * fixed updates of excluded prefixes Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * added metadata Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * addressed review changes, refactored unit tests Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * modified updating logic, added diff cutting of prefixes Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * returned old excludedPrefixServer implementation, added client Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * removed custom equality method Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
…k@main PR link: networkservicemesh/sdk#1180 Commit: eefb4a2 Author: Авраменко Михаил Date: 2021-12-24 19:34:52 +0700 Message: - fixed updates of excluded prefixes (#1180) * fixed updates of excluded prefixes Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * added metadata Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * addressed review changes, refactored unit tests Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * modified updating logic, added diff cutting of prefixes Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * returned old excludedPrefixServer implementation, added client Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * removed custom equality method Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
…k@main PR link: networkservicemesh/sdk#1180 Commit: eefb4a2 Author: Авраменко Михаил Date: 2021-12-24 19:34:52 +0700 Message: - fixed updates of excluded prefixes (#1180) * fixed updates of excluded prefixes Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * added metadata Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * addressed review changes, refactored unit tests Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * modified updating logic, added diff cutting of prefixes Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * returned old excludedPrefixServer implementation, added client Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * removed custom equality method Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
…k@main PR link: networkservicemesh/sdk#1180 Commit: eefb4a2 Author: Авраменко Михаил Date: 2021-12-24 19:34:52 +0700 Message: - fixed updates of excluded prefixes (#1180) * fixed updates of excluded prefixes Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * added metadata Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * addressed review changes, refactored unit tests Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * modified updating logic, added diff cutting of prefixes Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * returned old excludedPrefixServer implementation, added client Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * removed custom equality method Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
…k@main PR link: networkservicemesh/sdk#1180 Commit: eefb4a2 Author: Авраменко Михаил Date: 2021-12-24 19:34:52 +0700 Message: - fixed updates of excluded prefixes (#1180) * fixed updates of excluded prefixes Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * added metadata Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * addressed review changes, refactored unit tests Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * modified updating logic, added diff cutting of prefixes Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * returned old excludedPrefixServer implementation, added client Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * removed custom equality method Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
…k@main PR link: networkservicemesh/sdk#1180 Commit: eefb4a2 Author: Авраменко Михаил Date: 2021-12-24 19:34:52 +0700 Message: - fixed updates of excluded prefixes (#1180) * fixed updates of excluded prefixes Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * added metadata Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * addressed review changes, refactored unit tests Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * modified updating logic, added diff cutting of prefixes Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * returned old excludedPrefixServer implementation, added client Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * removed custom equality method Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
…k@main PR link: networkservicemesh/sdk#1180 Commit: eefb4a2 Author: Авраменко Михаил Date: 2021-12-24 19:34:52 +0700 Message: - fixed updates of excluded prefixes (#1180) * fixed updates of excluded prefixes Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * added metadata Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * addressed review changes, refactored unit tests Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * modified updating logic, added diff cutting of prefixes Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * returned old excludedPrefixServer implementation, added client Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * removed custom equality method Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
…k@main PR link: networkservicemesh/sdk#1180 Commit: eefb4a2 Author: Авраменко Михаил Date: 2021-12-24 19:34:52 +0700 Message: - fixed updates of excluded prefixes (#1180) * fixed updates of excluded prefixes Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * added metadata Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * addressed review changes, refactored unit tests Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * modified updating logic, added diff cutting of prefixes Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * returned old excludedPrefixServer implementation, added client Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * removed custom equality method Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
…k@main PR link: networkservicemesh/sdk#1180 Commit: eefb4a2 Author: Авраменко Михаил Date: 2021-12-24 19:34:52 +0700 Message: - fixed updates of excluded prefixes (#1180) * fixed updates of excluded prefixes Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * added metadata Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * addressed review changes, refactored unit tests Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * modified updating logic, added diff cutting of prefixes Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * returned old excludedPrefixServer implementation, added client Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * removed custom equality method Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
Signed-off-by: Mikhail Avramenko avramenkomihail15@gmail.com
Description
Excluded prefixes never removed, only appended. It's not desired behaviour.
Issue link
closes #1179
How Has This Been Tested?
Types of changes